-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add pod PriorityClass support #807
Add pod PriorityClass support #807
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the PR @gsnegovskiy !
Looks good. Just a few comments/suggestions.
- We like to document all options in the values.yaml. I understand there is no reasonable default for this though.
So I suggest adding something like the following false-y value as a default so nothing is rendered in the template.
## The PriorityClass of the ingress controller pods.
priorityClassName:
This would be similar to how controller.defaultTLS.secret is listed.
- I think it may make more sense to nest under controller.pod but I don't feel so strongly about this. What do ye think @Rulox @pleshakov ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gsnegovskiy thanks for the PR!
Lgmt pending @Dean-Coakley suggestion about the values.yaml file.
I think it may make more sense to nest under controller.pod but I don't feel so strongly about this. What do ye think @Rulox @pleshakov ?
I think it is fine as is and consistent with the other similar parameters like controller.tolerations
…egovskiy/kubernetes-ingress into add-priority-class-to-helm-templates
@Dean-Coakley @pleshakov Thank's for the review! Hopefully I've added documentation in a right place inside values.yaml Anything else I can do to fit standards ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good to go!
Thanks again for the PR!
Proposed changes
PriorityClass for pods is a stable feature since Kubernetes 1.14
It will be useful to have it available to configure priorities for ingress controller.